would the code be easier to generate if we jumped to (015) in case this instruction (007) was not a match?

the fact that we know we can skip the second or operand in this case is an optimization. Not saying it's not a good optimization, but if using a "fixed" offset makes code generation much easier, maybe we should do it for now.

I'm thinking about the discussion we just had during the community meeting. That being said, I can see why generating the same code as tcpdump is valuable.

@AryanBakliwal Please take a look at this ^

Taking a concrete example:

tcpdump -d "ip proto 6 and ((src host 10.244.1.2 and dst host 10.244.1.3) or (src host 10.244.1.3 and dst host 10.244.1.2))"

This is the code generated by tcpdump:

(000) ldh      [12]
(001) jeq      #0x800           jt 2	jf 12
(002) ldb      [23]
(003) jeq      #0x6             jt 4	jf 12
(004) ld       [26]
(005) jeq      #0xaf40102       jt 6	jf 8
(006) ld       [30]
(007) jeq      #0xaf40103       jt 11	jf 12
(008) jeq      #0xaf40103       jt 9	jf 12
(009) ld       [30]
(010) jeq      #0xaf40102       jt 11	jf 12
(011) ret      #262144
(012) ret      #0

If we generate this code instead, I believe it will be simpler:

(000) ldh      [12]
(001) jeq      #0x800           jt 2	jf 12
(002) ldb      [23]
(003) jeq      #0x6             jt 4	jf 12
(004) ld       [26]
(005) jeq      #0xaf40102       jt 6	jf 8
(006) ld       [30]
(007) jeq      #0xaf40103       jt 11	jf 8   <- less efficient than tcpdump's version but easier to implement?
(008) jeq      #0xaf40103       jt 9	jf 12
(009) ld       [30]
(010) jeq      #0xaf40102       jt 11	jf 12
(011) ret      #262144
(012) ret      #0

@antoninbas Actually, the real problem is the calculation of skipTrue (for which I created the function calculateSkipTrue) when we want to accept the packet. But calculateSkipTrue depends on many parameters [srcPort, dstPort, direction, which port is loaded in the register (srcPort or dstPort) and if we are calling this function from the first half (instructions that check request) or the second half (instructions that check response)] to calculate the number of instructions to skip to jump to the returnKeep instruction. This makes the code generation very complex if generating instruction set exactly like tcpdump.

Taking each case:

1. ICMP (both srcPort and dstPort are 0)

tcpdump -d "ip proto 6 and ((src host 10.244.1.2 and dst host 10.244.1.3) or (src host 10.244.1.3 and dst host 10.244.1.2))"

Code generated by tcpdump:

(000) ldh      [12]
(001) jeq      #0x800           jt 2	jf 12
(002) ldb      [23]
(003) jeq      #0x6             jt 4	jf 12
(004) ld       [26]
(005) jeq      #0xaf40102       jt 6	jf 8
(006) ld       [30]
(007) jeq      #0xaf40103       jt 11	jf 12    <- need to jump to 11 from 7, difference to calculate = 3
(008) jeq      #0xaf40103       jt 9	jf 12
(009) ld       [30]
(010) jeq      #0xaf40102       jt 11	jf 12    <- skipTrue = 0
(011) ret      #262144
(012) ret      #0

2. TCP (srcPort = 0 and dstPort > 0)

tcpdump -d "ip proto 6 and ((src host 10.244.1.2 and dst host 10.244.1.3 and dst port 80) or (src host 10.244.1.3 and dst host 10.244.1.2 and src port 80))"

Code generated by tcpdump:

(000) ldh      [12]
(001) jeq      #0x800           jt 2	jf 22
(002) ldb      [23]
(003) jeq      #0x6             jt 4	jf 22
(004) ld       [26]
(005) jeq      #0xaf40102       jt 6	jf 13
(006) ld       [30]
(007) jeq      #0xaf40103       jt 8	jf 22    <- jump to next instruction since port(s) not zero
(008) ldh      [20]
(009) jset     #0x1fff          jt 22	jf 10
(010) ldxb     4*([14]&0xf)
(011) ldh      [x + 16]
(012) jeq      #0x50            jt 21	jf 22    <- need to jump to 21 from 12, difference to calculate = 8
(013) jeq      #0xaf40103       jt 14	jf 22
(014) ld       [30]
(015) jeq      #0xaf40102       jt 16	jf 22
(016) ldh      [20]
(017) jset     #0x1fff          jt 22	jf 18
(018) ldxb     4*([14]&0xf)
(019) ldh      [x + 14]
(020) jeq      #0x50            jt 21	jf 22    <- skipTrue = 0
(021) ret      #262144
(022) ret      #0								 

Note that this is similar to when (srcPort > 0 and dstPort = 0) and (srcPort > 0 and dstPort > 0).

Since the problem is to jump to the returnKeep instruction, why not add it again to mitigate this complex calculation and jump to it easily. For example:

1. ICMP:
   Code that can be generated more simply

(000) ldh      [12]
(001) jeq      #0x800           jt 2	jf 12
(002) ldb      [23]
(003) jeq      #0x6             jt 4	jf 12
(004) ld       [26]
(005) jeq      #0xaf40102       jt 6	jf 8
(006) ld       [30]
(007) jeq      #0xaf40103       jt 8	jf 13    <- just skip to next instruction if true
(008) ret      #262144
(009) jeq      #0xaf40103       jt 10	jf 13
(010) ld       [30]
(011) jeq      #0xaf40102       jt 12	jf 13
(012) ret      #262144
(013) ret      #0

2. TCP (srcPort = 0 and dstPort > 0)
   Code that can be generated more simply

(000) ldh      [12]
(001) jeq      #0x800           jt 2	jf 23
(002) ldb      [23]
(003) jeq      #0x6             jt 4	jf 23
(004) ld       [26]
(005) jeq      #0xaf40102       jt 6	jf 13
(006) ld       [30]
(007) jeq      #0xaf40103       jt 8	jf 23
(008) ldh      [20]
(009) jset     #0x1fff          jt 23	jf 10
(010) ldxb     4*([14]&0xf)
(011) ldh      [x + 16]
(012) jeq      #0x50            jt 13	jf 23    <- just skip to next instruction if true
(013) ret      #262144
(014) jeq      #0xaf40103       jt 15	jf 23
(015) ld       [30]
(016) jeq      #0xaf40102       jt 17	jf 23
(017) ldh      [20]
(018) jset     #0x1fff          jt 23	jf 19
(019) ldxb     4*([14]&0xf)
(020) ldh      [x + 14]
(021) jeq      #0x50            jt 22	jf 23
(022) ret      #262144
(023) ret      #0								 

Let me know what you think about this approach

Adding an extra return instruction sounds fine to me.

I think what we are going for is some code that looks like this in the end:

if direction == SourceToDest {
        generateFilters(offset, srcIP, dstIP, srcPort, dstPort)
} else if direction == DestToSource {
        generateFilters(offset, dstIP, srcIP, dstPort, srcPort)
} else {
        generateFilters(offset, srcIP, dstIP, srcPort, dstPort)
        // ...
        generateFilters(offset, dstIP, srcIP, dstPort, srcPort)
}

So if we can get there and have some generate code that looks reasonable and works, that would be good.

@antoninbas I have made the requested changes and refactored the code as you suggested and as per our discussion. PTAL and let me know if there is anything else to update or add.

Maybe a stupid question, but if this is the only change we have, it doesn't seem worth it? Why can't we calculate the jump size using size - curOffset - 1 given that:

1. we always compute the full size up front
2. we always know the current offset ("where we are")
3. the instruction before last is always a MATCH return instruction

What am I missing?

@antoninbas You're right, the jump size can indeed be calculated easily with the available information (full size and current offset).
The main challenge, however, isn't the calculation itself but rather when to perform it, meaning in which instructions do we need skipTrue set to size - curOffset - 1.
And if a helper function is used for calculation, then it depends on which instruction is calling the helper function, what is the direction and what is the value of ports. Adding this extra instruction helps simplify the instruction set generation and avoids unnecessary complexity.

The challenges I faced:

For request packets in Both direction, there are 3 cases (assuming the 3 instructions - dstIP. srcPort and srcPort comparison instructions are calling a helper function to get jump size/skipTrue value):

• When both ports are 0, the destination IP comparison instruction performs the calculation to jump to MATCH.
• When only one port is 0, the destination IP comparison sets skipTrue to 0, and the comparison for the defined port performs the calculation to jump to MATCH.
• When neither port is 0, all comparisons set skipTrue to 0, except for the destination port comparison, which performs the calculation to jump to MATCH.

In SourceToDestination, DestinationToSource and response packets in Both, no calculations are needed, and these 3 instructions (dstIP, srcPort and dstPort comparison) jump to MATCH step with skipTrue set to 0.

The helper function that calculates the jump size or returns the value of skipTrue requires additional context which is a bunch of parameters such as the calling instruction, direction, ports, size and current offset. Adding this logic in the code makes it difficult to extend in the future if we filter traffic on more parameters and also increases complexity.

Hope that this explanation makes sense and adding an extra MATCH instruction seems reasonable. Any suggestions on how we could simplify or approach this differently? I'm open to any ideas you might have for improving the implementation of calculation of skipTrue value.

in comment section, else #26 should be else $27？

this function name is not very straightforward, can you use a new one? we may also need some comments.

Also i think the param useSkipFalse maybe misleading, we always use skip false, the difference is skipFalse to ret #0 or skipFalse to some intermediate instructions.